Skip to content

eslint: make --fix remove unused imports #1076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 6 commits into from
Nov 18, 2022
Merged

eslint: make --fix remove unused imports #1076

merged 6 commits into from
Nov 18, 2022

Conversation

@phillco
Copy link
Member

@phillco phillco commented Oct 23, 2022 •
edited
Loading

I noticed that eslint --fix didn't remove unused imports and eslint src --ext ts complained with a warning about an existing unused one:

$ eslint src --ext ts
.../src/core/commandVersionUpgrades/canonicalizeAndValidateCommand.ts
  1:10  warning  'window' is defined but never used. Allowed unused vars must match /^_/u  @typescript-eslint/no-unused-vars

I imagine this is because most people are fixing this up using the Visual Studio Code action, but it's nice to have everything working from the command line. Also JetBrains will only do it if eslint --fix does, hence my motivation :)

This adds the configuration such that eslint src --ext ts --fix removed unused imports, and fixes the existing unused imports in the codebase. Let me know if I made a mistake.

Checklist

@phillco phillco requested a review from pokey as a code owner October 23, 2022 18:26
@phillco phillco changed the title eslint: make --fix remove unused imports eslint: make --fix remove unused imports Oct 23, 2022
pokey
pokey reviewed Oct 24, 2022
View reviewed changes
Copy link
Member

@pokey pokey left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! I am definitely supportive of this direction

I think we need to tweak the ignore comments for most of these unused imports, though; see inline comment.

Also see #640; would be good to get this into pre-commit if easy

auscompgeek
auscompgeek reviewed Oct 24, 2022
View reviewed changes
@phillco
Copy link
Member Author

phillco commented Oct 24, 2022 •
edited
Loading

Yeah, I definitely did take a look at #640 :). A quick naive implementation had it making a bunch of changes, so I stopped for now.

Will update the tagging on the existing imports.

@phillco phillco requested a review from pokey October 31, 2022 22:24
@phillco
Copy link
Member Author

phillco commented Oct 31, 2022

I've tagged the existing unused imports with unused-imports/no-unused-imports and restored them, and updated the branch. Will take a look at https://github.com/import-js/eslint-plugin-import next to see if it's better, but otherwise this version is ready to go.

@phillco phillco requested a review from auscompgeek November 1, 2022 19:26
@phillco
Copy link
Member Author

phillco commented Nov 1, 2022

From the meetup: we looked at https://github.com/import-js/eslint-plugin-import briefly but didn't see a rule for removing unused imports. no-unused-modules seems to be more about finding exports in your own modules that aren't used. @auscompgeek let me know if I missed it.

If there is, we can use it. Otherwise, this should be ready to merge and we can take a look at adding some of eslint-plugin-import's rules as a follow up change, as I imagine that would be valuable in their own right.

@phillco
Copy link
Member Author

phillco commented Nov 6, 2022

@auscompgeek @pokey can we merge this?

@pokey pokey added the question Further information is requested label Nov 8, 2022
pokey
pokey approved these changes Nov 18, 2022
View reviewed changes
Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

@phillco phillco merged commit 371e74e into main Nov 18, 2022
@phillco phillco deleted the unused-imports branch November 18, 2022 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pokey pokey pokey approved these changes

@AndreasArvidsson AndreasArvidsson Awaiting requested review from AndreasArvidsson

@auscompgeek auscompgeek Awaiting requested review from auscompgeek

Assignees

@auscompgeek auscompgeek

Labels

question Further information is requested

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@phillco @auscompgeek @pokey